Add optional redirect loop protection to AuthenticationService by dereuromark · Pull Request #752 · cakephp/authentication

dereuromark · 2025-11-20T14:10:20Z

Summary

This PR adds optional redirect validation to AuthenticationService::getLoginRedirect() to prevent redirect loop attacks that have been observed in production environments.

Problem

The current implementation validates that redirect URLs are relative (not external) but does NOT check for:

❌ Nested redirect parameters
❌ Multiple encoding levels
❌ Redirects to authentication pages (causing loops)
❌ Excessively long URLs (DOS prevention)

Real-world evidence: Production logs show bots (especially GPTBot) creating 6-7 levels of nested redirects, wasting server resources and potentially enabling security exploits.

Solution

Adds configurable redirectValidation option with:

Opt-in design: Disabled by default for backward compatibility
Four validation checks: Redirect depth, encoding levels, URL length, blocked patterns
Extensible: Protected validateRedirect() method can be overridden
Well-tested: 8 new test cases, all existing tests pass (312 total)
Fully documented: New documentation page with security considerations

Configuration Example

$service->setConfig([
    'queryParam' => 'redirect',
    'redirectValidation' => [
        'enabled' => true,
        'maxDepth' => 1,
        'maxEncodingLevels' => 1,
        'maxLength' => 2000,
        'blockedPatterns' => ['#/login#i', '#/logout#i'],
    ],
]);

Changes

✅ Add validateRedirect() protected method to AuthenticationService
✅ Add redirectValidation configuration with sensible defaults
✅ Update getLoginRedirect() to call validation
✅ Add 8 comprehensive test cases (all pass)
✅ Add detailed documentation (docs/en/redirect-validation.rst)
✅ Maintain 100% backward compatibility (disabled by default)
✅ All existing tests pass (312 tests, 926 assertions)
✅ Code style checks pass

Validation Logic

Redirect depth: Count redirect= occurrences in decoded URL
Encoding level: Count %25 (percent-encoding of %)
URL length: Check total character count
Blocked patterns: Match against configured regex patterns

If validation fails, returns null instead of invalid URL.

Security Impact

Before:

⚠️ Nested redirects like /login?redirect=/login?redirect=/login...
⚠️ Double-encoded URLs like %252F
⚠️ Redirects to /logout causing loops
⚠️ Excessively long URLs (potential DOS)

After (when enabled):

✅ All nested redirects blocked
✅ Deep encoding detected and rejected
✅ Auth page loops prevented
✅ URL length limited

References

Issue: [Security] Add redirect loop protection to AuthenticationService #751
OWASP: Unvalidated Redirects and Forwards
CWE-601: URL Redirection to Untrusted Site

Testing

# Run new tests
vendor/bin/phpunit --filter testGetLoginRedirect

# Run full test suite
vendor/bin/phpunit

# Check code style
vendor/bin/phpcs --standard=vendor/cakephp/cakephp-codesniffer/CakePHP src/

Checklist

Ready for review! Happy to make any adjustments based on feedback.

- Add configurable redirect validation to prevent redirect loop attacks - Checks for nested redirects, deep encoding, blocked patterns, and URL length - Disabled by default for backward compatibility (opt-in) - Add comprehensive test coverage (8 new tests) - Add detailed documentation with security considerations - Fixes issue #751 Real-world evidence shows bots creating 6-7 levels of nested redirects, wasting server resources and potentially enabling security exploits. Configuration example: 'redirectValidation' => [ 'enabled' => true, 'maxDepth' => 1, 'maxEncodingLevels' => 1, 'maxLength' => 2000, 'blockedPatterns' => ['#/login#i', '#/logout#i'], ]

Copilot

Pull Request Overview

This PR adds optional redirect validation to AuthenticationService::getLoginRedirect() to prevent redirect loop attacks observed in production. The feature is disabled by default for backward compatibility and can be enabled via the redirectValidation configuration option.

Adds validateRedirect() protected method with four validation checks (depth, encoding, length, patterns)
Adds comprehensive test coverage with 8 new test cases
Provides detailed documentation with security considerations and usage examples

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/AuthenticationService.php	Implements the validateRedirect() method and adds redirectValidation configuration with defaults
tests/TestCase/AuthenticationServiceTest.php	Adds 8 comprehensive test cases covering disabled validation, nested redirects, encoding levels, blocked patterns, URL length, custom patterns, and query parameters
docs/en/redirect-validation.rst	New documentation page explaining the feature, configuration options, validation logic, and security considerations
docs/en/index.rst	Adds link to the new redirect-validation documentation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/AuthenticationService.php

docs/en/redirect-validation.rst

src/AuthenticationService.php

@ADmad

- Fix comparison operators: Use >= instead of > for maxDepth and maxEncodingLevels This correctly blocks URLs when they meet or exceed the threshold - Replace empty() with ! for enabled check (cleaner intent) - All tests still pass (312 tests, 926 assertions) Addresses feedback from @ADmad and @Copilot in PR #752

dereuromark · 2025-11-20T14:35:50Z

Thanks for the quick review @ADmad and @copilot!

I've addressed all the feedback in commit 1ae39c2:

✅ Fixed comparison operators (>= instead of >)

Changed redirectCount > maxDepth to >=
Changed encodingCount > maxEncodingLevels to >=
This correctly blocks when the count meets or exceeds the threshold

✅ Replaced empty() with !

Changed empty($config['enabled']) to !$config['enabled'] for clearer intent

All tests still pass (312 tests, 926 assertions). Ready for another review!

markstory

This whole thing seems quite complex for the problem it is trying to solve.

src/AuthenticationService.php

@markstory

Address maintainer feedback from @markstory and @ADmad: - Remove blockedPatterns configuration option - Remove pattern-based validation logic - Update documentation to show custom pattern validation in subclass - Remove 2 pattern-based tests (testGetLoginRedirectValidationBlockedPatterns, testGetLoginRedirectValidationCustomPatterns) Result: Simpler, focused implementation covering the core security issues: - Nested redirect detection - Deep encoding detection - URL length limits Custom pattern blocking can still be achieved by overriding validateRedirect(). All tests pass: 310 tests, 920 assertions Code style checks pass

dereuromark · 2025-11-20T16:57:31Z

Thanks for the feedback @markstory and @ADmad!

I've simplified the implementation in commit 591ec19:

Removed

❌ blockedPatterns configuration - Removed entirely
❌ Pattern-based validation logic - Removed from core implementation
❌ 2 pattern-based tests - Removed

What Remains (Core Security Features)

✅ Nested redirect detection - Blocks /login?redirect=/login?redirect=...
✅ Deep encoding detection - Blocks double-encoded URLs like %252F
✅ URL length limits - Prevents DOS via long URLs

Custom Pattern Blocking

Applications that need pattern-based blocking (e.g., blocking redirects to /login, /logout) can extend AuthenticationService and override validateRedirect(). I've updated the documentation with an example.

Result: Much simpler implementation focused on the core redirect loop vulnerability.

310 tests pass (920 assertions)
Code style checks pass
Documentation updated with custom validation example

Ready for another review!

dereuromark · 2025-11-21T21:07:29Z

Gonna test it on the server before we tag any release.

dereuromark · 2025-11-21T21:29:31Z

So far so good.

Only one more case where it can run into loops. when working together with authorization - and UnauthorizedHandler.
It seems there is one important part missing that gets triggered when a user logged in has not sufficient rights, when you there try to send a user to the action they wanted to go.

This seems to solve it, but its not really too nice.

// SafeRedirectHandler extends plugin one
	public function handle(Exception $exception, ServerRequestInterface $request, array $options = []): ResponseInterface {
		// If user is already logged in but not authorized, redirect to fallback
		// instead of login page (which would cause a loop)
		$identity = $request->getAttribute('identity');
		if ($identity) {
			$message = $options['unauthorizedMessage'] ?? __('You are not authorized to access that location.');
			if ($message) {
				/** @var \Cake\Http\ServerRequest $request */
				$session = $request->getSession();
				$session->write('Flash.flash', [
					[
						'message' => $message,
						'key' => 'flash',
						'element' => 'flash/error',
						'params' => [],
					],
				]);
			}

			$fallbackUrl = Router::url(['prefix' => false, 'plugin' => false, 'controller' => 'Account', 'action' => 'index']);

			return (new Response())
				->withHeader('Location', $fallbackUrl)
				->withStatus(302);
		}

dereuromark requested a review from Copilot November 20, 2025 14:15

Copilot started reviewing on behalf of dereuromark November 20, 2025 14:16 View session

Copilot finished reviewing on behalf of dereuromark November 20, 2025 14:18

Copilot AI reviewed Nov 20, 2025

View reviewed changes

src/AuthenticationService.php Outdated Show resolved Hide resolved

src/AuthenticationService.php Outdated Show resolved Hide resolved

docs/en/redirect-validation.rst Show resolved Hide resolved

ADmad reviewed Nov 20, 2025

View reviewed changes

src/AuthenticationService.php Outdated Show resolved Hide resolved

dereuromark added this to the 3.x milestone Nov 20, 2025

markstory reviewed Nov 20, 2025

View reviewed changes

src/AuthenticationService.php Outdated Show resolved Hide resolved

dereuromark merged commit 3e302f6 into cakephp:3.x Nov 21, 2025
8 checks passed

dereuromark deleted the feature/redirect-loop-protection branch November 21, 2025 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional redirect loop protection to AuthenticationService#752

Add optional redirect loop protection to AuthenticationService#752
dereuromark merged 3 commits intocakephp:3.xfrom
dereuromark:feature/redirect-loop-protection

dereuromark commented Nov 20, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dereuromark commented Nov 20, 2025

Uh oh!

markstory left a comment

Uh oh!

Uh oh!

dereuromark commented Nov 20, 2025

Uh oh!

Uh oh!

dereuromark commented Nov 21, 2025

Uh oh!

dereuromark commented Nov 21, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

dereuromark commented Nov 20, 2025

Summary

Problem

Solution

Configuration Example

Changes

Validation Logic

Security Impact

References

Testing

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dereuromark commented Nov 20, 2025

Uh oh!

markstory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dereuromark commented Nov 20, 2025

Removed

What Remains (Core Security Features)

Custom Pattern Blocking

Uh oh!

Uh oh!

dereuromark commented Nov 21, 2025

Uh oh!

dereuromark commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dereuromark commented Nov 21, 2025 •

edited

Loading